On Wed, Jul 20, 2022 at 8:21 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > Hi, > > I'd like to propose to remove "whichChkpt" and "report" arguments in ReadCheckpointRecord(). "report" is obviously useless because it's always true, i.e., there are two callers of the function and they always specify true as "report". Yes, the report parameter is obvious to delete. The commit 1d919de5eb removed the only call with the report parameter as false. > "whichChkpt" indicates where the specified checkpoint location came from, pg_control or backup_label. This information is used to log different messages as follows. > > switch (whichChkpt) > { > case 1: > ereport(LOG, > (errmsg("invalid primary checkpoint link in control file"))); > break; > default: > ereport(LOG, > (errmsg("invalid checkpoint link in backup_label file"))); > break; > } > return NULL; > ... > switch (whichChkpt) > { > case 1: > ereport(LOG, > (errmsg("invalid primary checkpoint record"))); > break; > default: > ereport(LOG, > (errmsg("invalid checkpoint record"))); > break; > } > return NULL; > ... > > But the callers of ReadCheckpointRecord() already output different log messages depending on where the invalid checkpoint record came from. So even if ReadCheckpointRecord() doesn't use "whichChkpt", i.e., use the same log message in both pg_control and backup_label cases, users can still identify where the invalid checkpoint record came from, by reading the log message. > > Also when whichChkpt = 0, "primary checkpoint" is used in the log message and sounds confusing because, as far as I recall correctly, we removed the concept of primary and secondary checkpoints before. Yes, using "primary checkpoint" confuses, as we usually refer primary in the context of replication and HA. > Therefore I think that it's better to remove "whichChkpt" argument in ReadCheckpointRecord(). > > Patch attached. Thought? How about we transform the following messages into something like below? (errmsg("could not locate a valid checkpoint record"))); after ReadCheckpointRecord() for control file cases to "could not locate valid checkpoint record in control file" (errmsg("could not locate required checkpoint record"), after ReadCheckpointRecord() for backup_label case to "could not locate valid checkpoint record in backup_label file" The above messages give more meaningful and unique info to the users. May be unrelated, IIRC, for the errors like ereport(PANIC, (errmsg("could not locate a valid checkpoint record"))); we wanted to add a hint asking users to consider running pg_resetwal to fix the issue. The error for ReadCheckpointRecord() in backup_label file cases, already gives a hint errhint("If you are restoring from a backup, touch \"%s/recovery.signal\" ...... Adding the hint of running pg_resetwal (of course with a caution that it can cause inconsistency in the data and use it as a last resort as described in the docs) helps users and support engineers a lot to mitigate the server down cases quickly. Regards, Bharath Rupireddy.